Conversation
Security ReviewClaude hit the max-turns limit or encountered an error before posting findings. See the workflow run for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4632 +/- ##
==========================================
+ Coverage 89.75% 89.85% +0.09%
==========================================
Files 444 445 +1
Lines 21661 21727 +66
==========================================
+ Hits 19442 19522 +80
+ Misses 2219 2205 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
josephjclark
left a comment
There was a problem hiding this comment.
Fantastic!
It feels a lot better to think about this stuff here than in the issue. The top banner is working great - setting per-group context really helps
Some minor tweaks in comments and maybe we'll want to tune the UI right at the end
|
@josephjclark ready for another look. Addressed the five inline comments. One wording call to flag: you'd suggested "Webhook security is disabled for sandboxes", but auth methods from the parent are still enforced on the sandbox's webhook triggers, so I went with "Webhook authentication is managed in the parent project" instead - happy to swap if you prefer yours. |
|
@josephjclark just a bit of nudge on this one too whenever you have 5 mins to QA. Also feel free to merge it in the Sandbox Collection branch |
|
Yep thanks @elias-ba - I'm at the end of my day now but I'll likely get this merged in the morning |
Make Project Settings behave differently inside a sandbox, per the categorization agreed in #3398: - Each tab shows a banner (via `SandboxSettingsBanner`) stating whether changes flow on merge: Local, Editable, or Inherited. - Sandbox Setup tab: identity card links back to the parent project; the danger zone's button is "Delete sandbox" and routes through `Sandboxes.delete_sandbox/2` with a name-typing confirm modal, landing on the root project's workflows page after delete. - Security tab: MFA toggle is read-only (value inherited from parent). - Webhook security tab: auth methods are managed from the parent project; the sandbox tab shows a notice + link back. - Credentials / Collections tabs: editable, flow on merge. - Collaboration tab: new parent-admin floor rule — a user who is admin or owner on any ancestor project cannot be removed from a sandbox (enforced in UI via disabled Remove button and server-side via `Projects.delete_project_user!/1` raising). - `root_project` assign added to the settings LV so the delete flow can redirect to the correct top-level ancestor. Also a small UX fix on the shared sandbox delete confirm modal: dropped the duplicate `<.errors>` row since the shared `<.input>` already renders field errors. Closes #3398.
c57fe73 to
3c34961
Compare
|
Ack sorry I had a commit with some tiny style changes which failed to push earlier. I've just pushed it up now |
# Conflicts: # CHANGELOG.md
Description
This implements the per-page approach we agreed on in #3398. The Project Settings page now behaves differently when you're inside a sandbox: each tab tells you whether your changes will sync on merge, and the UI for some tabs changes shape to match what makes sense for a sandbox (no Delete button, MFA read-only, webhook security managed at the parent level, etc.).
The categorization, agreed in the issue thread:
projectcredentialscollectionswebhook_securitycollaborationsecurity(MFA)vcsdata-storagehistory-exportsA few things worth knowing about how this is implemented, because they affected the scope:
The merge layer is already protective. I started worrying that "Local" and "Inherited" were just UI promises and that a user changing data retention in a sandbox would actually push that value back to the parent on merge. After tracing through
MergeProjects.merge_project/3andProvisioner.import_document/4, that turns out not to be the case. The merge document only takes[:name, :description, :env, :color]from the target (not the source), and the provisioner only casts[:id, :name, :description]on the project itself. Project-level fields likerequires_mfa,concurrency,retention_policy,history_retention_period,dataclip_retention_periodaren't in the cast list, so they never propagate. Webhook auth methods, project_users, and the GitHub repo connection aren't in the merge document at all. I added regression tests insandboxes_test.exsto lock this in so a future change to MergeProjects or the Provisioner doesn't accidentally start syncing them.The parent admin floor rule is enforced server-side, not just in the UI. A user who is admin or owner on any ancestor project (walking the parent chain, so admin on grandparent counts) cannot be removed from a sandbox. The UI disables the Remove button with a tooltip explaining why, but
Projects.delete_project_user!/1also raises anArgumentErrorif you try to do it directly, so a curl bypass or a future code path can't get around it. There's a unit test covering both paths.Webhook auth methods stay enforced but can't be managed from the sandbox. Auth methods flow down from the parent and are still applied to the sandbox's webhook triggers, but the sandbox UI points users back to the parent for create/edit/delete. The tab is still present so users can find it; the panel renders a notice and a link to the parent project's webhook security tab instead of the full management table.
Closes #3398
Validation steps
Setup
adminon this project.You're now inside the sandbox. Every test below happens from here unless I say otherwise.
1. Sandbox Setup tab (Local)
Navigate to Project Settings in the sandbox. The first tab should be Setup.
What you should see:
Sandboxes.delete_sandbox/2and redirects to the root project's workflows page with the flash "Sandbox and all its associated descendants deleted"2. Credentials and Collections (Editable)
These are the two tabs where changes flow back. We'll create one of each in the sandbox now and confirm in step 7 that they show up in the parent after merge.
Credentials
Collections
3. Webhook Security (managed in parent)
Click Webhook Security. Instead of the usual auth methods table, you should see a notice that reads something like:
Followed by a link: "Manage them in the parent project ([Cat Shelter Registry])" pointing back to the parent's webhook security tab.
4. Collaboration and the parent admin floor rule
Click Collaboration. You should see a Local banner.
The collaborators table shows the current sandbox members. The teammate you added as admin in step 3 of Setup should also be listed (they were inherited when the sandbox was provisioned).
Now hover over the Remove Collaborator button next to that admin. The button should be disabled, and the tooltip should read:
For comparison, add another teammate to the sandbox as
editor(not admin on the parent). Their Remove button should be enabled normally.5. Security tab (Inherited)
Click Security. You should see:
not-allowedon hover, the switch reflects the parent's value)The value you see should match what you set on the parent in step 2 of Setup.
6. The other Local tabs
Click through Sync to GitHub, Data Storage, History Exports. Each should show the blue Local banner at the top. The forms remain editable. This is intentional: you can change these in the sandbox to support sandbox-specific workflows, but the values do not sync back.
7. Merge: what flows back, what doesn't
This is the part that worried me when I started, so worth confirming hands-on.
Still in the sandbox:
Credentials (Editable — should sync):
Collections (Editable — should sync):
Setup (Local — should not sync):
Data Storage (Local — should not sync):
Security (Inherited — should not sync):
Editable categories flow back, Local and Inherited categories don't. That asymmetry is what
sandboxes_test.exslocks in at the merge-document/provisioner level.AI Usage
Pre-submission checklist
/reviewwith Claude Code)